Its funny -- I think that if we are going to make these elements composable, then we are in an unhappy medium here. The original meganode on the issue-1 branch was an attempt at dead simplicity for the user. The AutoCommutator workflow in this PR provides a similar level of useability, so that's covered. However, the TurnMotor sink feels kind of unnecessary to me. Right now it does three things

1. Provide access to write to a serial port. WriteSerialLine does this pretty effectively already
2. Formats a string. Expressions.Format seems like its fine for this already
3. Toss ridiculous turn commands (NaN or inifnity). I guess a custom expression script would be needed to do this filtering in Bonsai, so that's really the only major thing I see.

Aside from the limited gains from this node, what if we want more control over the Commuator state beyond turning the motor? e.g. what if we wanted to have some Property that provides control over the commutator's LED state using {led: false}. We could

1. Make a property in TurnMotor that modifies the string sent to the motor to include the the additional LED state (non-reactive since it changes when the turn command issued)
2. Make a subject in TurnMotor that merges its output sequence with turn commands
3. We could add a parallel stream in the AutoCommutator workflow that sent {led: false} to a SerialWriteLine with the same port as the TurnMotor. This feels hacky because PortName needs to be defined for both the TurnMotor node and the SerialWriteLine node.

These all feel kinda hacky in comparison to defining AutoCommuator as the following:

So my suggestion is to remove TurnMotor. If customizable control over the behavior of the commuator is what making composable nodes provides, then I think TurnMotor actually hurts in this regard.

@jonnew Thanks for clarifying the additional functionality of the commutator device. Given this I agree we should refactor the TurnMotor operator to be either a CommutatorDevice or simply Commutator and expose a more general interface to the entire device functionality.

Detailed reasons below:

Formats a string. Expressions.Format seems like its fine for this already

While it is possible to format the JSON string with the Format operator, this locks us to a specific communication protocol. If we ever need to upgrade or extend the details of the JSON API in any way (or swap the protocol to binary, etc), we would not be able to easily upgrade old workflows. With the operator abstracting away the communication internals, we can get away with just upgrading the package, even for hand-crafted workflows.

I don't currently see any particular advantage of exposing the internal JSON strings, especially given the protocol does not comply to an existing standard and there is little cost or complexity added in doing this abstraction.

Toss ridiculous turn commands (NaN or inifnity). I guess a custom expression script would be needed to do this filtering in Bonsai, so that's really the only major thing I see.

Input validation is definitely an important consideration. Similar to the above, having the packaged node would make it much easier to upgrade legacy workflows to benefit from improved validation. For example, in the future we could extend this operator to drop or throw if inputs are sent too fast, etc.

Aside from the limited gains from this node, what if we want more control over the Commuator state beyond turning the motor?

This is where having a general CommutatorDevice node would really make this interface shine and again allow us to extend the device or API with extra functionality by simply updating the package.

The easiest way to do this would have been for the commutator to use one of the standard control protocols already integrated into bonsai such as Harp or even ONI itself. This would have allowed us to leverage a bunch of existing infrastructure for configuration, control, logging and synchronization with other devices. In fact, if it was Harp we could even have generated the entire Bonsai interface automatically.

All the questions you asked about how to dynamically parameterize device configuration and having multiple control streams have already been solved for all Harp devices for example. I don't know what microcontroller the commutator uses, but there is now an existing Harp core for the RP2040 (https://github.com/AllenNeuralDynamics/harp.core.rp2040) which would have been perfect for this. A simple version could also be designed on top of other microcontroller architectures, including software forms of clock synchronization which have been implemented for other devices.

Assuming this is not possible for the current hardware / firmware combination:

Make a property in TurnMotor that modifies the string sent to the motor to include the the additional LED state (non-reactive since it changes when the turn command issued)

This is definitely a possibility and I think we could make this pattern work by using a BehaviorSubject in the backend. Whether this is appropriate depends on how big is the full API, and on whether a simple LED toggle is really the only extra functionality. If there are other configuration or control commands which would be useful to expose as streams (now or in the future) this might quickly become too complicated to evolve.

Make a subject in TurnMotor that merges its output sequence with turn commands

I'm not sure I understood this suggestion correctly, but I'm interpreting this to be the same pattern we use for the Harp Device node, which is basically a node which takes a sequence of commands and formats / pipes them to the serial port.

I think this would definitely be flexible enough for all present and future needs. Basically in this pattern we need just two concepts:

• A set of commutator nodes (TurnMotor, EnableLed, etc), which format inputs into the corresponding serializable JSON strings (or other protocol) to send to the serial port.
• A CommutatorDevice node which establishes a connection to the commutator and receives formatted strings.
• (optional) Make the "command" nodes output an abstract base class instead of strings, so that the CommutatorDevice node can declare a "strongly-typed" input. This would immediately disallow accidentally sending random or malformed strings to the device.

I think the last two bullet points would on their own also justify having a CommutatorDevice node.

Even though SerialWriteLine may suffice to send arbitrary strings to the serial port, this operator does not make any assumptions on port configuration properties such as baud rate, etc. Even if the commutator uses the default settings of the bonsai node for now, there is no guarantee this won't change in the future, either on the side of the commutator, or the side of bonsai, so relying on this alignment to be stable in the future is dangerous IMO. Having a common device node would ensure the alignment is enforced and the port configured correctly.

The last point is optional but easy to implement and important for validation to ensure the commutator does not go into an undefined state by accidentally receiving gibberish data from upstream.

We could add a parallel stream in the AutoCommutator workflow that sent {led: false} to a SerialWriteLine with the same port as the TurnMotor. This feels hacky because PortName needs to be defined for both the TurnMotor node and the SerialWriteLine node.

This could actually be done very easily by linking both properties via the same ExternalizedMapping. However, I do agree that it is still hacky because of all the issues discussed above.

These all feel kinda hacky in comparison to defining AutoCommuator as the following:

Seeing the manual control of the motor with keys and LED control functionality added to the AutoCommutator workflow actually makes me feel that what we should really remove is the AutoCommutator node itself.

It feels there are currently two things being conflated in this package:

1. The commutator device is actually a complex device, so we need all of its functionality to be exposed in a structured way. This is so we can freely compose what we want to do with it in arbitrary ways, e.g. reset commutator on trials, blink LED to signal something, apply a slower drift correction algorithm, untwist commutator with computer vision tracking, etc the list will just keep growing.

2. There is a need for a "plug-and-play" commutator solution that users can drop into their acquisition workflows to get started quickly doing some recordings.

I feel that 2. could just as easily be provided simply as an example in the commutator docs, with no need for an embedded workflow. This recommended example usage workflow could be updated regularly as we improve the pattern without requiring updates to the commutator package.

That said, I don't disagree we could also make it into an embedded workflow in the package, although in that case I feel it should be fairly minimalistic, e.g. without assuming specific keys or options, since the more we assume the higher is the risk that it will not work for someone's specific workflow.